Always use CE_CACHE, remove TYPE_HAS_CE #7336
Conversation
ext/opcache/ZendAccelerator.c
Outdated
| finish: | ||
| /* Transfer CE_CACHE map ptr slot to new interned string. | ||
| * Should only happen for permanent interned strings with permanent map_ptr slot. */ | ||
| if (ZSTR_HAS_CE_CACHE(str)) { | ||
| ZEND_ASSERT(GC_FLAGS(str) & IS_STR_PERMANENT); | ||
| GC_SET_REFCOUNT(s, GC_REFCOUNT(str)); | ||
| GC_ADD_FLAGS(s, IS_STR_CLASS_NAME_MAP_PTR); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is only necessary for calls from accel_copy_permanent_strings(). Right?
There was a problem hiding this comment.
I think that's the only place it can happen, though it shouldn't hurt to check for it in other cases either.
This is missing a && !ZSTR_HAS_CE_CACHE(s) check though -- no need to do this again if cache is already present.
| if (zend_string_equals_literal_ci(type_name, "self") | ||
| || zend_string_equals_literal_ci(type_name, "parent")) { |
There was a problem hiding this comment.
It may make sense to add self and parent into ZEND_KNOWN_STRINGS and compare just pointers.
| zend_string *name = zend_new_interned_string(ZEND_TYPE_NAME(*single_type)); | ||
| ZEND_TYPE_SET_PTR(*single_type, name); | ||
| zend_alloc_ce_cache(name); |
There was a problem hiding this comment.
This won't allocate new cache slot for eval()'ed code when opcache active (or when opcache is full).
| if (ZSTR_HAS_CE_CACHE(type_name) || !ZSTR_IS_INTERNED(type_name)) { | ||
| return; | ||
| } | ||
|
|
||
| if ((GC_FLAGS(type_name) & IS_STR_PERMANENT) && startup_done) { |
There was a problem hiding this comment.
This means, that we won't allocate CE cache slots during requests if opcache active. Right?
This will affect eval()'ed code, opcache.blacklisted files and situation when opcache is full.
There was a problem hiding this comment.
Yes, this is the key tradeoff with this patch. There are cases where we can't allocate CE_CACHE (eval with opcache, permanent strings during request without opcache), so property types will be uncached (arg types still have separate cache slot).
I didn't have any simple ideas on how to address this. One thought I had is that we could change the map_ptr mechanism to not work using "ptr or offset", but to have two different map_ptr_base for permanent and per-request and always use offset. That is *(map_ptr_base[ptr & 1] + ptr) as the access method. Per-request slots would then be allocated in the map_ptr area rather than on CG arena.
This would allow allocating new per-request slots without clashing with new permanent slots allocated by opcache. However, just that isn't enough as well, as with opcache there are no per-request interned strings at all, and we can't use the refcount as storage for non-interned strings.
Do you have any ideas?
There was a problem hiding this comment.
It's possible to grow map_ptr vector in both directions having the single biased base. e.g. positive offsets for permanent slots and negative for per-request slots. anyway, this is not for 8.1
phpdbg can perform module startup + shutdown more than once.
Currently, CE_CACHE on strings is only used opcache interned strings. This patch extends usage to non-opcache interned strings as well. This means that most type strings can now make use of CE_CACHE even if opcache is not loaded, which allows us to remove TYPE_HAS_CE kind, and fix some discrepancies depending on whether a type stores a resolved or non-resolved name.
There are two cases where CE_CACHE will not be used:
In these two cases we would end up not using CE caching for property types (argument/return types still have the separate cache slot).